Conversation
In-memory jobs with two-step admission and a slot reservation; abortable preparation with task-budget and deadline timers; a synchronous launch check before D's start; first-reason stops that keep the slot until D settles; Store-chosen terminal states; unresolved markers on storage failure; cancel task; and shutdown that keeps existing reasons and waits for every job. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Sep 26, 2026
mchwang
added a commit
that referenced
this pull request
Sep 26, 2026
* docs: reconcile the design plan with the code Record where the code differs from the approved plan and update stale status: - Record Ask as an interim exception to R1: it runs the vendor CLI on the host with tools off until lane F moves it into the lane D container. - Amend D20: there is no "Merge anyway"; to override a blocker, merge on GitHub. Matches docs/implementation/guarded-merge.md. - Tick T1, T2, T4, T5, T10, T13, T14 with test evidence; point Files lines at core/linking.ts and core/approvals.ts instead of never-created modules. - Mark increment 1 merged; add a lane status table (C, D, K done; E, F, H progress); record decided open questions (issue ranking, AgentDiff). - Add a verified status note for design tasks DT2-DT15; none newly ticked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: note merge-queue support in the guarded merge doc The guarded merge gate doc still said merge-queue branches stay blocked. #46 (closing #24) added queue lifecycle support. Point to merge-queue.md, and state that adapters without queue inspection still fail closed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: bring plan status up to date with F1 and H4a Merge main. Record the F1 lifecycle contract (#49) and open F1a-F1c (#53, #56, #57); record the ranked Issues screen (H4a, #55) with H4b's trust action remaining; Issues is now a menu link, not a placeholder. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: add F1d and Ask PRs to lane status Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Align merge-queue wording with the merged K2/K3 support README no longer says merge queues block merging; it describes the enqueue-then-confirm behaviour. The plan's wave-3 note records the old block as history instead of a live instruction. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Record the K-lane queue block as history in the task table Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * README: disclose that Ask runs the agent CLI on the host The plan (R1 exception) says README states this limit; it did not. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Add F1e (#60) and the #51 merge condition to the F lane row Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * README: distinguish queue-removal retry from changed-head review Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Plan: mark the install preflight and npx entry as planned The CLI checks only the Node version today; say so instead of describing the git/gh/container/sign-in preflight as current. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
mchwang
added a commit
that referenced
this pull request
Sep 27, 2026
* docs: reconcile the design plan with the code Record where the code differs from the approved plan and update stale status: - Record Ask as an interim exception to R1: it runs the vendor CLI on the host with tools off until lane F moves it into the lane D container. - Amend D20: there is no "Merge anyway"; to override a blocker, merge on GitHub. Matches docs/implementation/guarded-merge.md. - Tick T1, T2, T4, T5, T10, T13, T14 with test evidence; point Files lines at core/linking.ts and core/approvals.ts instead of never-created modules. - Mark increment 1 merged; add a lane status table (C, D, K done; E, F, H progress); record decided open questions (issue ranking, AgentDiff). - Add a verified status note for design tasks DT2-DT15; none newly ticked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: note merge-queue support in the guarded merge doc The guarded merge gate doc still said merge-queue branches stay blocked. #46 (closing #24) added queue lifecycle support. Point to merge-queue.md, and state that adapters without queue inspection still fail closed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Run Ask in the lane D agent container Ask used to run the claude/codex CLI on the host with each CLI's own restrictions, an interim exception to R1. It now uses lane D's invocation boundary in the read-only "questions" phase: a clone of the reviewed snapshot head at /work, no commands, vendor-only network, and no other host files. There is no host fallback. - runner/question-container.ts: build image, clone, allocate bounded storage, capture, start the Claude/Codex adapter; release storage only after the invocation settles. Deps are injectable for unit tests. - runner/question-worker.ts: lane D setup is synchronous, so a worker thread owns it and the review server stays responsive. - runner/question-agent.ts: QuestionWorker bridge; a question settles only when the worker reports the container and storage are gone. - Credentials come from the environment only: CLAUDE_CODE_OAUTH_TOKEN for Claude, CODEBOOST_CODEX_AUTH_FILE or CODEX_HOME/auth.json for Codex. - Provider failures include the vendor's short message (e.g. a 401). - test/agent-question.test.ts runs the path on real Docker (Agent isolation workflow); its live case needs the auth-probe credentials. - Plan, README, Settings copy and implementation docs updated; the R1 exception is closed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: bring plan status up to date with F1 and H4a Merge main. Record the F1 lifecycle contract (#49) and open F1a-F1c (#53, #56, #57); record the ranked Issues screen (H4a, #55) with H4b's trust action remaining; Issues is now a menu link, not a placeholder. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: add F1d and Ask PRs to lane status Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Align merge-queue wording with the merged K2/K3 support README no longer says merge queues block merging; it describes the enqueue-then-confirm behaviour. The plan's wave-3 note records the old block as history instead of a live instruction. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Record the K-lane queue block as history in the task table Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * README: disclose that Ask runs the agent CLI on the host The plan (R1 exception) says README states this limit; it did not. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Add F1e (#60) and the #51 merge condition to the F lane row Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Bind Ask answers to their attempt and keep cleanup ownership - Reuse the persisted answer attempt as the invocation attempt, and the note's contextId as referencedCodeHash. Accept a result only when its attempt and context match the captured invocation and the worker reply carries the same attempt. - Treat a missing exit code or any signal as a failure, not an answer. - Keep task storage whose removal Docker did not confirm, retry removal before the next question, and refuse Ask while any remains. - After a worker crash, fail closed instead of starting a replacement: its containers and storage may still exist, and reclaiming them needs lane D's scoped recovery (#51 item 4). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Run the Docker Ask suite when runner/questions.ts changes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Record Ask storage left at shutdown and keep Ask off until it is gone Terminating the question worker dropped its only handles to storage that Docker had not removed. Shutdown now asks the worker for one last bounded removal, records anything still unremoved beside the review database, and the next session refuses Ask, with the removal commands, while any recorded container or volume still exists. The record clears itself once they are gone; an unreadable record or unreachable daemon keeps Ask off. Removal through D waits for its recovery handles (#51). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Keep Ask off after a setup failure that leaves unidentifiable storage When task storage setup fails and lane D cannot confirm its own cleanup, D returns no handle, so Ask cannot name the leftovers. Ask now counts the failure, stays off for the session, records it at shutdown, and after a restart stays off while any io.codeboost.task-storage container or volume exists. Caller-provided allocation IDs (#51 item 3) would let Ask name these resources instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Make the Ask leftover gate bounded and fail closed on unknown state - A worker crash, or no release report at shutdown, is recorded at once as unidentified leftovers instead of an empty, clean release. - The pre-question check is two label queries (docker ps, docker volume ls) under one 15-second limit that the question's signal can cancel, instead of up to 300 sequential inspects. - Entries beyond the record's cap become unidentified leftovers; none are dropped. - Removal commands list only resources that still exist, so a missing keeper no longer blocks volume removal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Bound Ask against unsettled lane D cleanup and scan all D labels - Scan containers, volumes and networks for every label lane D applies (allocation, invocation, egress), so a leftover seeder or proxy keeps Ask off. - The first question of each process scans even without a record, so a process killed before writing one cannot bypass the gate. - A question not settled 30 s after its deadline, or still settling after a 20 s shutdown grace, abandons the worker: unknown leftovers are recorded, waiters rejected and the worker stopped, so D's unbounded cleanup retries (#51 item 1) cannot hang Ask or shutdown. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Own the host staging directory like the Docker allocation If the host copy of the reviewed code cannot be deleted, the worker now keeps its path and retries before the next question, shutdown records it, and the next leftover check deletes it. Ask stays off while any copy remains. The record accepts only codeboost-question-* staging paths. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Keep credentials out of setup subprocesses and settle abandon in order - The question worker snapshots credentials for the adapters and removes credential-like variables from its own environment, so the image build, clone and other setup subprocesses cannot inherit them. Leftover Docker queries use lane D's minimal PATH/DOCKER_HOST environment. - Missing sign-in is reported before the leftover scan or any Docker work. - Abandoning a worker records unknown leftovers, then waits (bounded) for the thread to stop before rejecting its questions, so their slots stay owned until a synchronous Docker or Git call has returned. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Own every host copy through a recorded Ask root; fix CI env dependency - The bridge creates one Ask root per worker (<tmp>/codeboost-ask-*), records it before the worker starts, and runs the worker with it as TMPDIR, so the reviewed clone, lane D's input directory and its Codex auth copy all live inside it. The root is deleted after the thread stops (clean shutdown, crash or abandon); otherwise the next check deletes it, and Ask stays off while an earlier root remains. - The record accepts only direct children of the real temp directory named codeboost-ask-XXXXXX, so a lookalike path elsewhere is refused instead of deleted. - Test fix: the bridge checks sign-in before asking, so the stub worker now gets its own Codex auth file instead of depending on ~/.codex. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Delete an abandoned worker's root once its thread finally stops If the bounded wait for an abandoned worker ends while its thread is still inside a synchronous Docker or Git call, its ownership is already durable (unknown leftovers and the recorded Ask root) and no new question is admitted. The root is now also deleted, and dropped from the record, as soon as that thread does stop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Serialize Ask per review with a lock and bound the release timeout - Take an exclusive per-review Ask lock (PID lock file next to the leftover record) before the startup scan and hold it until the worker has stopped; only the holder scans, starts a worker or writes the record. A lock left by a dead process is taken over. - A worker that does not answer the final release request now goes through the bounded abandon path instead of an unbounded terminate, keeping its root and the lock until the thread stops. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Test that a refused second process cannot delete a live Ask root Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Give the Ask worker an allowlisted environment; credentials as data Replace the name-based credential scrub with an explicit allowlist: the worker's environment is only PATH, DOCKER_HOST and its Ask root as TMPDIR, so every setup subprocess (including the image build) inherits no credentials, home directory, Docker config or agent socket. The credential lookup's four variables reach the worker via workerData and go only to the adapters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Use an OS lock for Ask and key it by the canonical database path - Replace the PID file and liveness takeover with an exclusive SQLite transaction on the lock file: an OS file lock the operating system releases when its process ends, so PID reuse cannot let two holders overlap and no takeover is needed. - Key the lock and leftover record by the database's realpath, so relative, absolute and symlinked spellings share them; refuse Ask on a database with other hard links. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Clean up the Ask root when the worker cannot be constructed If the Worker constructor throws after the root was created and recorded, delete the root and drop it from the record, so close() can release the per-review Ask lock. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Serialize Ask worker abandonment and bound the host clone up front - All abandon triggers (crash, watchdogs, shutdown) share one bounded termination promise, so a second trigger cannot reject questions and free their slots while the thread is still in a synchronous call. - Before lane D's unbounded host clone, measure the checkout at the reviewed head and the object store with Git plumbing and refuse a repository that would not fit the question's storage allocation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Never drop recorded Ask roots; keep the marker for unnamed resources - Ask roots are never sliced from the record; recording one past the cap is refused, which also refuses to start another worker. - Any labelled Docker resource that is not part of a still-listed allocation (a seeder, agent container, proxy or network) keeps the unidentified marker after the named entries are gone, and it clears only when none remain. Named removal commands are reported first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Make the Ask startup scan single-flight Concurrent first questions now share one startup scan instead of each running their own, so a second scan cannot see the first question's new labelled resources and record them as earlier-session leftovers. Each caller can stop waiting through its own signal, and a failed scan is retried by the next question. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Wait for an in-progress abandonment when the Ask worker closes If a crash or watchdog is already abandoning the worker when shutdown calls close(), close() now awaits that bounded settlement instead of returning at once, so the thread, its recorded root and the lock are settled before Questions.close() finishes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Key the Ask lock by file identity; finish the startup scan under it - The lock file is keyed by the database's device and inode in the temp directory, so every spelling and every later name of the file, including an atomic rename while a server runs, finds the same lock. The durable record stays next to the canonical database path. - close() waits for a shared startup scan still in flight before releasing the lock, so the scan cannot write the record unlocked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Find Ask roots by their owner stamp, not only through the record The durable record sits beside the database path, so after a rename a new process would not see roots recorded under the old name. Each Ask root now carries an .owner stamp naming its lock, written under a preparation name before the folder is renamed into place. The first check of a process deletes unrecorded codeboost-ask-* folders whose owner lock is free and leaves those whose owner is still running. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Harden the Git size check and trust only codeboost lock stamps - The pre-clone Git measurement now uses the same hardening as lane D's clone: GIT_NO_LAZY_FETCH, protocol.allow=never, no replace objects, no hooks, no graft file, no submodule recursion. - An .owner stamp is probed only when it names a codeboost lock file in the temp directory; anything else counts as no owner, so a lookalike root cannot make startup open or create a file elsewhere. Every ledger's lock now lives there under that name. - Regression test: startup still scans Docker after deleting a recorded root (the reported bypass does not reproduce). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Name the labels the Ask leftover scan actually checks The docs and one Ask error message still said io.codeboost.task-storage, but the scan checks containers, volumes and networks labelled io.codeboost.allocation, io.codeboost.invocation or io.codeboost.egress. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Write the Ask leftover record through an exclusive random temp file The record was written via a predictable <record>.<pid>.tmp name with the default "w" flag, so a planted link at that name would be followed and its target overwritten. Use a random name opened with "wx" and delete it if the write or rename fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Stop Ask admission when shutdown begins; keep the root if recording fails - The server stops question admission in the same turn it starts shutting down, so a request still arriving cannot start an agent or a container worker during the drain. A question it saved gets a retryable "Server stopped" answer without any agent starting; the existing drain test now expects zero agent calls instead of one started-then-cancelled call. - If the final release report cannot be saved, the worker's root is no longer deleted: it stays on disk and in the record for the next session, and Docker leftovers remain covered by the startup scan. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Release the Ask lock when the final record write fails After a failed release-report write the worker thread has stopped and the root is already recorded, so let go of the root in this process: it stays on disk and in the record for the next check, and close() can release the per-review lock instead of holding it for the process. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Treat a failed worker termination as not stopped A rejected terminate() no longer counts as a stopped thread: the Ask root stays on disk and in the record, and the lock stays held, instead of being removed while the worker may still be alive. Later cleanup runs only after a termination that actually settled. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Add AGENTS.md rules from the PR #54 review-lesson audit Four rules for owned host and Docker resources: durable cleanup ownership, allowlisted subprocess environments and credential channels, untrusted record and on-disk paths, and cross-process OS locks keyed by stable identity. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Close the store on failed Ask cleanup; private locks; keep lock after abandon - web/server.ts closes the review store in a finally block when Ask's cleanup fails, and the CLI exits non-zero instead of hanging. - Ask lock files live in a private per-user directory under the temp directory (mode 0700, checked ownership); a lock path that is a symlink or not a plain file is refused, never opened. - After any abandonment the review lock is kept until the process exits: Docker CLI children the terminated thread started can outlive it and cannot be awaited until lane D exposes process groups (#51). - AGENTS.md: the untrusted-path rule also forbids following links and requires a private directory for plantable files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Pass the Ask stop reason as a typed value; leave foreign Ask folders alone - Closes #64: the stop reason (timeout, shutdown, cancelled) now travels as a StopError value from Questions through the worker message to handle.cancel(), instead of being rebuilt from message wording. - The orphan-root scan deletes only folders this user owns that carry a valid createAskRoot stamp naming a lock in the private lock directory whose owner is gone; unstamped, tampered or foreign folders stay. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Describe which Ask folders the orphan scan deletes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Read the Ask record and owner stamps without following links The leftover record and each folder's .owner stamp are now read through O_NOFOLLOW and accepted only as regular, single-link files within a size limit. A linked record makes the ledger unreadable (Ask fails closed and never acts on the record it points to); a linked stamp leaves the folder alone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lane F, step F1, slice F1b: runner coordinator. Stacked on #53 (F1a, the Store); merge #53 first. Related: #22, #51.
What this does
Adds
runner/coordinator.ts, the per-process owner of in-memory jobs, slots and unresolved markers, as specified indocs/implementation/runner-lifecycle.md. D is injected (RunnerDeps.start), so this runs against a fake D until #51 lands; nothing is wired to the server yet.Store.admitAttemptruns. A refused transaction releases the reservation in the same turn. Writable kinds share one slot and read-only kinds share another; both limits are configurable.AbortSignal. The task budget timer recordstime-limit. The attempt deadline, before launch, fails the attempt with "Timed out" and records no first reason.pending, there is no first reason (saved or in memory), there is time left and the context is current. ThencaptureInvocation, then D's start. A context change during preparation endsstalewithout calling D. A stop during launch ends from its first reason. A start error endsfailedwith the launch error.markRunningis refused because a first reason exists, the handle is cancelled and settles normally. If the write fails with a storage error, the handle is cancelled withcapture-failure, settled, and the slot stays held under astart-not-savedmarker.status().stopRequested.savedshows when saving failed. The slot is never freed on cancel, only aftersettled.result-not-savedmarker that holds the slot until restart.cancelTaskgoes through the Store and stops the running work.close()rejects admission, recordsshutdownonly where no reason exists, and awaits every job with no timer. It leaves the Store open for the caller to close.Validation (head
3767065)npm run typecheck: passes.CI's unit set (
npm testminus the Docker suites): 488 passed, 0 failed. 18 of those tests are new, intest/runner-coordinator.test.ts, using a fake D whose promises the test controls, and real SQLite.Mutation check: six guards were broken one at a time, and each was caught:
close()not waiting;AGENTS.md race regressions covered here:
The Store-level cases (an old attempt settling after a retry, two processes admitting at once) are in F1a.
Browser tests weren't rerun: this slice doesn't touch
web/.Not in this slice
/api/runner, and the merge-coordinator catches.preparation_pgidis part of F1d, together with the abortable D helpers from D follow-ups required by the F1 runner lifecycle contract #51.🤖 Generated with Claude Code